-
Notifications
You must be signed in to change notification settings - Fork 0
New app #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: gh-pages
Are you sure you want to change the base?
Conversation
stu-k
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of the Router! There was a little issue with the inputs on the login page because they weren't being set properly, but I made a note about that. Otherwise, you HAVE to make your code look nicer. Logically, this is fine code. Stylistically, this was a mess. React doesn't see whitespace, so having empty lines within JSX is useless. You're still using both 2 and 4 spaces, which on its own is a big no-no, but I also found lines with an ODD number of spaces. Fix the login page thing and all the unnecessary whitespace/horrible indentation and I'll give you a better grade.
| import RaisedButton from 'material-ui/RaisedButton'; | ||
|
|
||
|
|
||
| const HomePage = () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's with the horrible indentation?
| @@ -0,0 +1,16 @@ | |||
| import React from 'react'; | |||
| import LoginForm from '../forms/LoginForm'; | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary whitespace.
|
|
||
| const LoginPage = () => ( | ||
| <div> | ||
| <h1>Login page</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation.
| <h1>Login page</h1> | ||
|
|
||
| <LoginForm /> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space.
| value={data.password} | ||
| onChange={this.onChange} | ||
| /><br /> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space.
|
|
||
| <br /> | ||
|
|
||
| </Form> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be in-line with top <Form>
| hintText="email" | ||
| value={data.email} | ||
| onChange={this.onChange} | ||
| /><br /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be easier to follow if the <br /> was on the next line.
| data: { | ||
| email: '', | ||
| password: '' | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation. This makes it look like state ends here.
|
|
||
|
|
||
| class LoginForm extends React.Component { | ||
| state = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using both 4 spaces 2 here, don't.
| //in state change data, use spread to save everything that is in data, then what is changed | ||
| //which is an e.target | ||
| onChange = e => this.setState({ | ||
| data: {...this.state.data, [e.target.name]: e.target.value} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.target doesn't have a .name property, so you'll probably be unable to change the input values through typing in them. You could do e.target.dataset.name and then also give your TextFields a data name (<TextField data-name="email">) to achieve the same effect.
stu-k
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay intentions! Now whitespace and fix your login page onChange.
08week/newapp/src/App.js
Outdated
| return ( | ||
| // this is a semantic-ui-react also semantic-ui-css container that auto margins | ||
| <div className="ui container"> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space
| hintText="email" | ||
| value={data.email} | ||
| onChange={this.onChange} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space
| value={data.email} | ||
| onChange={this.onChange} | ||
|
|
||
| /><br/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<br /> On new line
| hintText="password" | ||
| value={data.password} | ||
| onChange={this.onChange} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space
| value={data.password} | ||
| onChange={this.onChange} | ||
|
|
||
| /><br/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<br /> on new line
| <RaisedButton primary={true}> | ||
| <Link to='main'>LOGIN</Link> | ||
| </RaisedButton> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space
| <h1>Home Page</h1> | ||
| <RaisedButton primary={true}> | ||
| <Link to='login'>LOGIN PAGE</Link> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space
| const MainPage = () => ( | ||
| <div> | ||
| <h1>Main page</h1> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space
|
|
||
| <img className="pic" src="http://www.simonstalenhag.se/bilderbig/by_procession_1920.jpg"> | ||
| </img> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White space
| //in state change data, use spread to save everything that is in data, then what is changed | ||
| //which is an e.target | ||
| onChange = e => this.setState({ | ||
| data: {...this.state.data, [e.target.name]: e.target.value} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some instances of both 4 spaces and 2, but you get the idea!
Edit: DON'T MERGE. Your branches are weird enough as is.
Checkpoint Rubric
This is the rubric that your instructor will use to grade your checkpoints. Please do not edit.
Checkpoint 1
Checkpoint 2
Checkpoint 3